Explicitly bind req/res to context domain in express requestHandler #269
Conversation
|
I noticed you put a fix in for node 0.10, this and 0.12 aren't supported by the node core team https://github.com/nodejs/LTS no harm in adding in this case since it's just a test case but in general might not be worth the effort of supporting node < v4 |
|
Yea, we're certainly not holding back any new development/capabilities for the sake of node < v4, but we know we have a number of users still on those older versions (despite node's guidelines/pleading), so we support them as long as it's not a blocker/overly obnoxious to do so (which thus far it hasn't been). The plan is roughly that when async_hooks shows up in node core, we'll do a new major version that supports only versions of node having async_hooks, and keep the older major version maintenance-only for everything pre-async_hooks. |
| e.on('done', function () { | ||
| // Context won't propagate here without the explicit binding of req/res done in the middleware | ||
| setTimeout(function () { | ||
| client.getContext().breadcrumbs.length.should.equal(1); |
There was a problem hiding this comment.
I assume that breadcrumbs[0].mesage==='test breadcrumb' here? is there a chance that some other one got added and this doesn't work?
There was a problem hiding this comment.
Yea, good call, I added an explicit check for that.
There was a problem hiding this comment.
I followed along #266, did some refreshing on Node's domains documentation, re-read this about 3 times and feel it is sound / accurately covered by the test case.
LewisJEllis
force-pushed
the
bug/gh-266
branch
from
604594e to
146c474
Compare
Managed to repro/test that this fixes issue in #266. The test case is a little tricky but hopefully makes sense.
/cc @benvinegar @MaxBittker @rwky